refactor(logging): rename llama_stack logger categories#3065
Conversation
50b5ffe to
91cd285
Compare
91cd285 to
96fbb8e
Compare
|
How have you checked the file base for missing alignment of the log category with the package name? Can you please show me your vibe coding context & prompt? When I use I find quite a few more spots (actually 27 instead of 5). Can you check again, please? Also, can you please specify which "rule" you used to map the package name (file path)? I'm assuming you always use the first path component, but it sometimes seems that the category is randomly picked from* some* path component of the Python file. It should be consistent and unambiguous, so that there is a clear rule also for the future, which category name to pick.
|
| from llama_stack.log import get_logger | ||
|
|
||
| logger = get_logger(name=__name__, category="auth") | ||
| logger = get_logger(name=__name__, category="core") |
There was a problem hiding this comment.
what is the rule to extract the category ? First path element ("core") or shouldn't it be last path element ("server") ?
There was a problem hiding this comment.
so I initially use the first path element, but over some reviews on the original PR, I have changed them accordingly :)
| from llama_stack.providers.utils.responses.responses_store import ResponsesStore | ||
|
|
||
| logger = get_logger(name=__name__, category="openai_responses") | ||
| logger = get_logger(name=__name__, category="agents") |
There was a problem hiding this comment.
Which "agents" (randomly from within the package name) and not "providers" (first path element in the package) ?
|
It's also ok to introduce a more complex rule-set like:
Please propose such a rule-set. It shouldn't be too involved, though. Try to keep it less than 5 rules. |
thanks for your review so from the table, I conclude that the correct category, is the outer package name, (e.g. for |
thanks for your guidance please find my proposal below
ptal 👍🏽 |
|
thanks for the proposal
"if relevant" should never be part of any rule, as there is a lot of leeway in interpretation about what is actually "relevant". You could say:
However, I would be equally acceptable to handpick a selection and not rely solely on the file path. For that, one should define the full list of categories. Perhaps it's better not to connect the package name, as I don't see any regularity right now that could be leveraged to fully determine the log category from the package / directory name. |
+1, I will remove it above, it should be clear to avoid confusion in the future 👍🏽
+1, therefore I sometimes used the package name and other use the top-level package name. I would appreciate input from others in order to have consensus over the categories names used now, and in the future cc @leseb @ashwinb @mattf @nathan-weinberg @skamenan7 @derekhiggins @bbrowning @cdoern @ehhuang |
|
As Per @ashwinb review in #3065 (comment)
This is the final proposal, i will apply it to #3061 now |
2257036 to
e0cbe5d
Compare
|
@rhuss updated according to the table in #3065 (comment) All changes here impacts loggers which is already in the code base, before merging #3061 I propose to merge this first, so that I can rebase #3061 before updating the categories. thanks |
f991345 to
1bad897
Compare
7515c9c to
c97dc1c
Compare
ashwinb
left a comment
There was a problem hiding this comment.
I am good with this PR mostly I have a couple of comments inline mostly the addition of a file which feels like a mistake.
| from llama_stack.providers.utils.kvstore.config import KVStoreConfig, SqliteKVStoreConfig | ||
|
|
||
| logger = get_logger(__name__, category="core") | ||
| logger = get_logger(__name__, category="core::store") |
There was a problem hiding this comment.
just name this core::registry
| ) | ||
|
|
||
| logger = get_logger(name=__name__, category="responses") | ||
| logger = get_logger(name=__name__, category="agents::meta_reference") |
There was a problem hiding this comment.
I think this should stay responses really, maybe call this openai::responses. this is why we cannot take these dictums too formally sometimes.
| @@ -0,0 +1,1154 @@ | |||
| # Copyright (c) Meta Platforms, Inc. and affiliates. | |||
There was a problem hiding this comment.
why is this file added? is that a bad rebase?
There was a problem hiding this comment.
yes, the commit is older than removing this file
thanks for catching this 👍🏽
7a98003 to
23ef375
Compare
This PR renames logging categores of llama_stack logger across the codebase according to #3065 (comment). Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>
23ef375 to
430ea87
Compare
# What does this PR do? <!-- Provide a short summary of what this PR does and why. Link to relevant issues if applicable. --> This PR renames categories of llama_stack loggers. This PR aligns logging categories as per the package name, as well as reviews from initial ogx-ai#2868. This is a follow up to ogx-ai#3061. <!-- If resolving an issue, uncomment and update the line below --> <!-- Closes #[issue-number] --> Replaces ogx-ai#2868 Part of ogx-ai#2865 cc @leseb @rhuss Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>
What does this PR do?
This PR renames categories of llama_stack loggers.
This PR aligns logging categories as per the package name, as well as reviews from initial #2868. This is a follow up to #3061.
Replaces #2868
Part of #2865
cc @leseb @rhuss